[analytics-engine] Fix CHECKED_LONG_SUM conversion by ahkcs · Pull Request #22611 · opensearch-project/OpenSearch

ahkcs · 2026-07-30T17:52:31Z

Description

SQL PR opensearch-project/sql#5612 introduced the reflective CHECKED_LONG_SUM aggregate. The analytics planner accepts it through SqlKind.SUM, but Isthmus resolves Substrait bindings using the exact Calcite operator object and therefore could not bind it.

This change adds a distinct checked_long_sum Substrait/runtime binding for aggregate and window calls. Its DataFusion UDAF delegates signatures, return types, grouped accumulators, and sliding accumulators to native SUM, preserving existing analytics semantics and performance.

Keeping the function name distinct also avoids duplicate intermediate schema fields when a plan contains both SUM(field) and CHECKED_LONG_SUM(field), such as AVG and SUM over the same column.

Testing

cargo test --lib native_and_checked_sum_keep_distinct_names
:sandbox:plugins:analytics-backend-datafusion:test --tests '*DataFusionFragmentConvertorTests'
TopKCssCorrectnessIT.testCase08_avgSum_cssMatchesNoCss
TopKCssCorrectnessIT.testCase10_noAliases_cssMatchesNoCss
TopKCssCorrectnessIT.testCase11_manyAggsOnSameColumn_cssMatchesNoCss
StatsCommandIT.testStatsIntegralSum
StreamstatsCommandIT.testMultipleStreamstatsWithEval
StreamstatsCommandIT.testMultipleStreamstatsWithEval2
:sandbox:plugins:analytics-backend-datafusion:precommit

github-actions · 2026-07-30T17:53:31Z

PR Reviewer Guide 🔍

(Review updated until commit `d0cc903`)

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ No major issues detected

github-actions · 2026-07-30T17:53:53Z

PR Code Suggestions ✨

Latest suggestions up to d0cc903

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use rebound call for aggregation checks The post-bind literal-rewrite logic uses the original `call` to check for `LocalAggOp`, but after `bindCheckedLongSum` the aggregation may have changed to `LOCAL_CHECKED_LONG_SUM_OP`. Use `substraitCall.getAggregation()` instead to ensure consistent behavior for rebound calls. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [772-778] - RexNode toConvert = call.getAggregation() instanceof LocalAggOp localOp + RexNode toConvert = substraitCall.getAggregation() instanceof LocalAggOp localOp ? localOp.normaliseLiteralArg(i, rexLit, rexBuilder, typeFactory) : rexLit; rewritten.set(i, rexConverter.apply(toConvert)); } if (rewritten == null) return bound; return Optional.of(ImmutableAggregateFunctionInvocation.builder().from(fn).arguments(rewritten).build()); } Suggestion importance[1-10]: 4 __ Why: The suggestion correctly notes that after `bindCheckedLongSum`, the `substraitCall` should be used consistently. However, since `LOCAL_CHECKED_LONG_SUM_OP` is not a `LocalAggOp`, the branch behavior is the same (falls through to `rexLit`). It's a minor consistency improvement with limited practical impact.	Low

Previous suggestions

Suggestions up to commit a01e3bd

Category	Suggestion	Impact
General	Clarify and harden SUM rebinding guard The guard condition is inverted: it short-circuits (returns `call` unchanged) for the native `SqlStdOperatorTable.SUM` operator, which is correct, but the `\|\|` with `getKind()` `!= SqlKind.SUM` means any operator whose kind is not `SUM` also skips rebinding — that's fine, but the intent reads incorrectly. More importantly, this also skips rebinding when the aggregation is already `LOCAL_CHECKED_LONG_SUM_OP`; verify that identity check is intentional. Consider using explicit checks for clarity and correctness. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [775-778] private AggregateCall bindCheckedLongSum(AggregateCall call) { - if (call.getAggregation() == SqlStdOperatorTable.SUM \|\| call.getAggregation().getKind() != SqlKind.SUM) { + SqlAggFunction op = call.getAggregation(); + if (op == SqlStdOperatorTable.SUM \|\| op == LOCAL_CHECKED_LONG_SUM_OP \|\| op.getKind() != SqlKind.SUM) { return call; } Suggestion importance[1-10]: 4 __ Why: Adding an explicit check for `LOCAL_CHECKED_LONG_SUM_OP` is a minor defensive improvement. Since `LOCAL_CHECKED_LONG_SUM_OP` has kind `SqlKind.SUM` but is not `SqlStdOperatorTable.SUM`, it would currently be rebinding to itself, which is a subtle correctness/efficiency concern.	Low
General	Skip rebinding for already-bound window op Also short-circuit when the operator is already `LOCAL_CHECKED_LONG_SUM_OP` to avoid unnecessarily rebuilding the `RexOver` (and to prevent potential infinite reprocessing if this converter is ever revisited). Same rationale as the aggregate path. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [818-823] private RexOver bindCheckedLongSum(RexOver call) { - if (call.getAggOperator() == SqlStdOperatorTable.SUM \|\| call.getAggOperator().getKind() != SqlKind.SUM) { + SqlAggFunction op = call.getAggOperator(); + if (op == SqlStdOperatorTable.SUM \|\| op == LOCAL_CHECKED_LONG_SUM_OP \|\| op.getKind() != SqlKind.SUM) { return call; } RexWindow window = call.getWindow(); return (RexOver) new RexBuilder(typeFactory).makeOver( Suggestion importance[1-10]: 4 __ Why: Same rationale for the window path — avoids unnecessary rebuild if the op is already `LOCAL_CHECKED_LONG_SUM_OP`. A minor robustness/efficiency improvement.	Low

Suggestions up to commit 6d8cc59

Category	Suggestion	Impact
General	Guard against re-binding already-bound operator The condition's operator precedence makes the first clause redundant and inverted: since `SqlStdOperatorTable.SUM.getKind() == SqlKind.SUM`, the second clause already excludes it. However, more importantly, the intent appears to be "return call unchanged if it is native SUM OR not a SUM kind at all", but the current logic is correct only by accident. Consider rewriting for clarity to avoid future regressions. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [775-778] private AggregateCall bindCheckedLongSum(AggregateCall call) { - if (call.getAggregation() == SqlStdOperatorTable.SUM \|\| call.getAggregation().getKind() != SqlKind.SUM) { + if (call.getAggregation() == SqlStdOperatorTable.SUM + \|\| call.getAggregation() == LOCAL_CHECKED_LONG_SUM_OP + \|\| call.getAggregation().getKind() != SqlKind.SUM) { return call; } Suggestion importance[1-10]: 3 __ Why: `LOCAL_CHECKED_LONG_SUM_OP` has `SqlKind.SUM`, so it would pass through the current guard and be re-bound. Adding an explicit check for `LOCAL_CHECKED_LONG_SUM_OP` provides minor robustness, but in practice this converter is unlikely to receive an already-bound call.	Low
General	Make window binding idempotent Similar to the aggregate case: also skip when the operator is already `LOCAL_CHECKED_LONG_SUM_OP` to make the binding idempotent and avoid rebuilding a new `RexOver` unnecessarily if this converter is invoked more than once. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [818-821] private RexOver bindCheckedLongSum(RexOver call) { - if (call.getAggOperator() == SqlStdOperatorTable.SUM \|\| call.getAggOperator().getKind() != SqlKind.SUM) { + if (call.getAggOperator() == SqlStdOperatorTable.SUM + \|\| call.getAggOperator() == LOCAL_CHECKED_LONG_SUM_OP + \|\| call.getAggOperator().getKind() != SqlKind.SUM) { return call; } Suggestion importance[1-10]: 3 __ Why: Same as suggestion 1: the idempotency guard is a marginal robustness improvement, mostly defensive rather than fixing an actual bug in the current flow.	Low

Suggestions up to commit b798c51

Category	Suggestion	Impact
Possible issue	Preserve original window flags on rewrite Hard-coding `allowPartial=true` and `nullWhenCountZero=false` discards the original window's semantics from `call`/`window`. This can silently change behavior for windows that were created with different flags. Preserve the original values from the source `RexOver` / `RexWindow` instead of replacing them with constants. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [802-816] return (RexOver) new RexBuilder(typeFactory).makeOver( call.getType(), SqlStdOperatorTable.SUM, call.getOperands(), window.partitionKeys, window.orderKeys, window.getLowerBound(), window.getUpperBound(), window.getExclude(), window.isRows(), - true, + window.isAllowPartial(), false, call.isDistinct(), call.ignoreNulls() ); Suggestion importance[1-10]: 6 __ Why: Valid observation that hard-coding `allowPartial` and `nullWhenCountZero` may lose original window semantics; preserving these from the source `RexOver` improves correctness, though impact may be limited given typical usage.	Low

Suggestions up to commit 2d8457e

Category	Suggestion	Impact
Possible issue	Derive return type from standard SUM The return type of the custom SUM (e.g. `CHECKED_LONG_SUM` returns BIGINT nullable) may differ from what `SqlStdOperatorTable.SUM` would infer from the same operands, so forcing `call.getType()` can produce a Substrait binding whose declared type doesn't match the standard SUM signature and causes downstream resolution failures. Consider deriving the type from `SqlStdOperatorTable.SUM` via `rexBuilder.deriveReturnType` (or the standard `makeCall`) rather than reusing the custom operator's type. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [802-816] -return (RexOver) new RexBuilder(typeFactory).makeOver( - call.getType(), +RexBuilder rb = new RexBuilder(typeFactory); +RelDataType sumType = rb.deriveReturnType(SqlStdOperatorTable.SUM, call.getOperands()); +return (RexOver) rb.makeOver( + sumType, SqlStdOperatorTable.SUM, call.getOperands(), window.partitionKeys, window.orderKeys, window.getLowerBound(), window.getUpperBound(), window.getExclude(), window.isRows(), true, false, call.isDistinct(), call.ignoreNulls() ); Suggestion importance[1-10]: 5 __ Why: Reasonable concern that the custom operator's return type may not match standard SUM's inferred type, potentially causing binding issues. However, the aggregate canonicalization path also reuses `call.getType()` and tests pass, so the impact is uncertain.	Low
General	Clarify canonicalization guard conditions The guard short-circuits when the aggregation is already `SqlStdOperatorTable.SUM`, but the operator precedence means it also short-circuits for any non-SUM kind after evaluating the OR. More importantly, the intent is "if it's already the standard SUM, or not a SUM at all, skip". The current logic is correct but fragile; consider making the condition explicit for readability and to avoid future misreads. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [755-757] private AggregateCall canonicalizeAggregate(AggregateCall call) { - if (call.getAggregation() == SqlStdOperatorTable.SUM \|\| call.getAggregation().getKind() != SqlKind.SUM) { + if (call.getAggregation() == SqlStdOperatorTable.SUM) { + return call; + } + if (call.getAggregation().getKind() != SqlKind.SUM) { return call; } Suggestion importance[1-10]: 2 __ Why: Minor stylistic refactor for readability; the original condition is logically correct and equivalent. Low impact.	Low

Suggestions up to commit dba238c

Category	Suggestion	Impact
General	Clarify canonicalization guard ordering The guard short-circuits when the aggregation is already `SqlStdOperatorTable.SUM`, but uses `\|\|` with a second condition that excludes non-SUM kinds. This is correct, but the logic is confusing and could be misread. More importantly, comparing operators by reference identity (`==`) is fragile; use `.equals()` or check that the operator is not already the canonical one to make intent clearer and safer. sandbox/plugins/analytics-backend-datafusion/src/main/java/org/opensearch/be/datafusion/DataFusionFragmentConvertor.java [755-757] private AggregateCall canonicalizeAggregate(AggregateCall call) { - if (call.getAggregation() == SqlStdOperatorTable.SUM \|\| call.getAggregation().getKind() != SqlKind.SUM) { + if (call.getAggregation().getKind() != SqlKind.SUM \|\| call.getAggregation() == SqlStdOperatorTable.SUM) { return call; } Suggestion importance[1-10]: 2 __ Why: The suggestion merely reorders the two conditions in an `\|\|` guard without changing behavior. The claim about `==` vs `.equals()` is not addressed in the improved code, and reference equality is appropriate for singleton operators like `SqlStdOperatorTable.SUM`.	Low

Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-07-30T17:58:22Z

Persistent review updated to latest commit 2d8457e

github-actions · 2026-07-30T18:45:28Z

Persistent review updated to latest commit b798c51

github-actions · 2026-07-30T19:09:16Z

❌ Gradle check result for b798c51: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

github-actions · 2026-07-30T20:15:26Z

Persistent review updated to latest commit 6d8cc59

Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-07-30T20:41:28Z

Persistent review updated to latest commit a01e3bd

github-actions · 2026-07-30T21:08:17Z

❌ Gradle check result for a01e3bd: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

…m-substrait

Signed-off-by: Kai Huang <ahkcs@amazon.com>

github-actions · 2026-07-31T16:43:58Z

Persistent review updated to latest commit d0cc903

github-actions · 2026-07-31T17:40:22Z

✅ Gradle check result for d0cc903: SUCCESS

codecov · 2026-07-31T17:40:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 71.36%. Comparing base (bacf3f6) to head (d0cc903).

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #22611      +/-   ##
============================================
- Coverage     71.39%   71.36%   -0.04%     
+ Complexity    76808    76741      -67     
============================================
  Files          6148     6148              
  Lines        357994   357994              
  Branches      52179    52179              
============================================
- Hits         255607   255495     -112     
- Misses        82054    82100      +46     
- Partials      20333    20399      +66

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sandeshkr419

Thanks @ahkcs for working on this. I have an alternative proposal which I suspect should simplify the changes.

The operator rewrite should belong in the RBO planner layer (a new OpenSearchCheckedLongSumRule modeled on OpenSearchDistinctCountRule), not inside DataFusionFragmentConvertor. Moving it earlier, i.e., before OpenSearchAggregateRule runs means the entire stack sees SqlStdOperatorTable.SUM by operator identity: AggregateSplitRule, TopK, the Lucene backend, and Isthmus all resolve it correctly without any special-casing.

The custom LOCAL_CHECKED_LONG_SUM_OP, YAML bindings, and checked_long_sum.rs Rust UDAF are only needed in this PR because the current rewrite happens too late (inside the fragment convertor, after Isthmus dispatch). With an early planner rewrite, DataFusion receives sum in the Substrait proto and resolves to its own native sum_udaf — the delegation wrapper in checked_long_sum.rs is pure boilerplate with zero custom logic and could be deleted entirely.

ahkcs requested a review from a team as a code owner July 30, 2026 17:52

Fix checked long sum analytics conversion

2d8457e

Signed-off-by: Kai Huang <ahkcs@amazon.com>

ahkcs force-pushed the fix/checked-long-sum-substrait branch from dba238c to 2d8457e Compare July 30, 2026 17:56

alchemist51 approved these changes Jul 30, 2026

View reviewed changes

Merge branch 'main' into fix/checked-long-sum-substrait

b798c51

sandeshkr419 changed the title ~~Fix CHECKED_LONG_SUM conversion in analytics engine~~ [analytics engine] Fix CHECKED_LONG_SUM conversion Jul 30, 2026

sandeshkr419 changed the title ~~[analytics engine] Fix CHECKED_LONG_SUM conversion~~ [analytics-engine] Fix CHECKED_LONG_SUM conversion Jul 30, 2026

Fix checked long sum schema collisions

a01e3bd

Signed-off-by: Kai Huang <ahkcs@amazon.com>

ahkcs force-pushed the fix/checked-long-sum-substrait branch from 6d8cc59 to a01e3bd Compare July 30, 2026 20:39

ahkcs added 2 commits July 31, 2026 09:39

Merge remote-tracking branch 'upstream/main' into fix/checked-long-su…

8d9630e

…m-substrait

Limit checked sum operator rebinding

d0cc903

Signed-off-by: Kai Huang <ahkcs@amazon.com>

sandeshkr419 requested changes Jul 31, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[analytics-engine] Fix CHECKED_LONG_SUM conversion - #22611

[analytics-engine] Fix CHECKED_LONG_SUM conversion#22611
ahkcs wants to merge 5 commits into
opensearch-project:mainfrom
ahkcs:fix/checked-long-sum-substrait

ahkcs commented Jul 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 30, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

codecov Bot commented Jul 31, 2026

Uh oh!

sandeshkr419 left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

ahkcs commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing

Uh oh!

github-actions Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit d0cc903)

Uh oh!

github-actions Bot commented Jul 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 30, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

github-actions Bot commented Jul 31, 2026

Uh oh!

codecov Bot commented Jul 31, 2026

Codecov Report

Uh oh!

sandeshkr419 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ahkcs commented Jul 30, 2026 •

edited

Loading

github-actions Bot commented Jul 30, 2026 •

edited

Loading

(Review updated until commit `d0cc903`)

github-actions Bot commented Jul 30, 2026 •

edited

Loading

sandeshkr419 left a comment •

edited

Loading